Turning this rule on as a testcase to see just how bad it is. Part of this will be updating our plugins and configs to define more globals automatically, the rest will be fixing the bugs that are uncovered.

While working on turning on no-undef I discovered that the various rules we have for defining globals are a little inconsistent in whether the files they load recurse through import-globals-from directives and none of them imported eslint globals directives. I think we're better off putting all this global parsing code in a single place rather than spread across multiple rules. Have one rule to turn it on for parsed files and one function to load globals from other files and make them share most of the code so we won't get inconsistent. If we find us needing to turn on/off individual features we can figure out a way to do that in the future. This patch does that, the globals.js file does all global parsing with a shared object that receives events from the AST, either through from an ESlint rule or from a simple AST walker using estraverse. Review commit: https://reviewboard.mozilla.org/r/34199/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34199/

To properly lint XBL files we need to support things like import-globals-from and other ESlint comment directives so we have to pass comments through to the code blocks that ESlint parses. Unfortunately the way the XBL processor works now is by passing a separate code block for every method/property/etc. in the XBL and ESlint doesn't retain state across the blocks so we would have to prefix every block with every comment. Instead this change makes us output just a single block that roughly looks like this: <comments> var bindings = { "<binding-id>": { <binding-part-name>: function() { ... } } } This has some interesting bonuses. Defining the same ID twice will cause a lint failure. Same for the same field in a binding. The line mapping is a little harder and there are still a few lines that won't map directly back to the original file but they should be rare cases. The only downside is that since some bindings have the same binding declared differently for different platforms we have to exclude those from linting for now. Review commit: https://reviewboard.mozilla.org/r/34201/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34201/

This adds more of the scripts that browser.js relies on and also makes browser-chrome head files import the browser.js globals. The MOZ_JSDOWNLOADS block in contentAreaUtils only seems to hide a single function, I don't see any need to keep hiding that now we're on by default. Review commit: https://reviewboard.mozilla.org/r/34203/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34203/

This defines a few additional globals but also turns on the browser environment for everything in browser and toolkit. This may lead to some false negatives but we have lots of code that runs in a browser context so in the name of getting rules turned on I think this is a useful step. Review commit: https://reviewboard.mozilla.org/r/34205/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34205/

xpcshell tests used to use head_*.js files so this adds those for global discovery. Review commit: https://reviewboard.mozilla.org/r/34207/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34207/

Mostly just declaring globals that Cu.imports defines but there are some actual bugs here that have been fixed as well as one test that just never ran because of a hidden exception. Review commit: https://reviewboard.mozilla.org/r/34209/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34209/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer https://reviewboard.mozilla.org/r/34209/#review30889 ::: toolkit/mozapps/extensions/AddonManager.jsm:981 (Diff revision 1) > + /*globals RemotePages*/ I've seen both this (implicitly importing the exported globals from a jsm) and alternatively: `let {RemotePages} = Cu.import("resource://gre/modules/RemotePageManager.jsm", {});` Not a deal-breaker for me as long as it's made explicit in some way (such as the /*globals*/ comment for eslint), just wondering if there's an advantage to not using an explicit `let`/`const`/etc. binding. ::: toolkit/mozapps/extensions/content/extensions.xml:12 (Diff revision 1) > +<!-- import-globals-from extensions.js --> So cool we can do this in XUL files too

Comment on attachment 8717537 [details] MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe https://reviewboard.mozilla.org/r/34203/#review30925

Comment on attachment 8717538 [details] MozReview Request: Bug 1245916: Add additional default globals. r=felipe https://reviewboard.mozilla.org/r/34205/#review30927

Comment on attachment 8717535 [details] MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset https://reviewboard.mozilla.org/r/34199/#review31177 Just a few comments and questions. Overall looks like a really good cleanup. ::: .eslintrc:7 (Diff revision 1) > - "mozilla/components-imports": 1, > + "mozilla/import-globals": 1, Did I miss something or are Cu.import, loader, XPCOMUtils things not going to be recognized as globals now? ::: testing/eslint-plugin-mozilla/lib/globals.js:17 (Diff revision 1) > +const globalCache = new Map(); nit: please add a line comment explaining what this globalCache is used for. ::: testing/eslint-plugin-mozilla/lib/globals.js:20 (Diff revision 1) > + * An object that returns found globals for given AST node types. Each property s/Eeach property/Each prototype property Just makes it easier when reading, you know which property you're talking about. ::: testing/eslint-plugin-mozilla/lib/globals.js:36 (Diff revision 1) > + if (match) { We've taken the habit, in the devtools code base, to invert these conditions and early return, so that the "actual" code wouldn't be indented, and more easily readable. Up to you. ``` if (!match) { return []; } value = match[1].trim(); let filePath = value; ... ``` ::: testing/eslint-plugin-mozilla/lib/globals.js:39 (Diff revision 1) > + let filePath = value; Why set `value` again here? Why not just `let filePath = match[1].trim();` ::: testing/eslint-plugin-mozilla/lib/globals.js:92 (Diff revision 1) > + // comment directives Isn't there a way to import an eslint helper here that would do this for us? ::: testing/eslint-plugin-mozilla/lib/globals.js:95 (Diff revision 1) > + let match = /^globals?\s+(.+)$/.exec(value); eslint accepts both `globals` and `global` ::: testing/eslint-plugin-mozilla/lib/globals.js:100 (Diff revision 1) > + for (let global of value.split(/\s*,\s*/)) { Turns out eslint also allows globals to be separated by a space character.

Comment on attachment 8717539 [details] MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset https://reviewboard.mozilla.org/r/34207/#review31183 ::: testing/eslint-plugin-mozilla/lib/helpers.js:311 (Diff revision 1) > * e.g. helpers.getIsBrowserMochitest(this) s/getIsBrowserMochitest/getIsHeadFile ::: testing/eslint-plugin-mozilla/lib/helpers.js:327 (Diff revision 1) > + * e.g. helpers.getIsBrowserMochitest(this) s/getIsBrowserMochitest/getIsXpcshellTest

Comment on attachment 8717536 [details] MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker https://reviewboard.mozilla.org/r/34201/#review31193

https://reviewboard.mozilla.org/r/34199/#review31177 > Did I miss something or are Cu.import, loader, XPCOMUtils things not going to be recognized as globals now? No they all still apply through GlobalsForNode.prototype.ExpressionStatement.

https://reviewboard.mozilla.org/r/34199/#review31177 > Turns out eslint also allows globals to be separated by a space character. I've just copied the function from eslint.js for this.

https://reviewboard.mozilla.org/r/34199/#review31239 ::: testing/eslint-plugin-mozilla/lib/globals.js:92 (Diff revision 1) > + // comment directives Not that I can see, it all happens at https://github.com/eslint/eslint/blob/master/lib/eslint.js#L270 and the only way to really get there is to just do a full eslint pass on the file. I guess that's an option but will involve more work than we need for now. ::: testing/eslint-plugin-mozilla/lib/globals.js:95 (Diff revision 1) > + let match = /^globals?\s+(.+)$/.exec(value); The ? after the "s" allows for that.

https://reviewboard.mozilla.org/r/34209/#review30889 > I've seen both this (implicitly importing the exported globals from a jsm) and alternatively: > `let {RemotePages} = Cu.import("resource://gre/modules/RemotePageManager.jsm", {});` > > Not a deal-breaker for me as long as it's made explicit in some way (such as the /*globals*/ comment for eslint), just wondering if there's an advantage to not using an explicit `let`/`const`/etc. binding. let bindings are probably better since they work even outside of eslint parsing. They don't have the same effect though. It doesn't actually matter here but inside a block the declaration would only be for the block rather than globally. Mostly for now I don't want to get into changing those that need it to let bindings because then I'd be tempted to change them all to let bindings and those are cycles I'd rather not spin right now!

Comment on attachment 8717535 [details] MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/1-2/

Comment on attachment 8717536 [details] MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/1-2/

Comment on attachment 8717537 [details] MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/1-2/

Comment on attachment 8717538 [details] MozReview Request: Bug 1245916: Add additional default globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/1-2/

Comment on attachment 8717539 [details] MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/1-2/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/1-2/

Comment on attachment 8717535 [details] MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset https://reviewboard.mozilla.org/r/34199/#review31399 Looks good thanks. And thanks for comment 13, I understand now.

Comment on attachment 8717535 [details] MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/2-3/

Comment on attachment 8717536 [details] MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/2-3/

Comment on attachment 8717537 [details] MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/2-3/

Comment on attachment 8717538 [details] MozReview Request: Bug 1245916: Add additional default globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/2-3/

Comment on attachment 8717539 [details] MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/2-3/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/2-3/

https://hg.mozilla.org/integration/fx-team/rev/878db4caf845 https://hg.mozilla.org/integration/fx-team/rev/01675e4828b5 https://hg.mozilla.org/integration/fx-team/rev/b554c7ce41c4 https://hg.mozilla.org/integration/fx-team/rev/04c1740aa499 https://hg.mozilla.org/integration/fx-team/rev/70eca07367f4 https://hg.mozilla.org/integration/fx-team/rev/3e5b6df276a9

Backout: https://hg.mozilla.org/integration/fx-team/rev/51577bed9ade

And backed out the rest in https://hg.mozilla.org/integration/fx-team/rev/8bd1a25ac261 for browser-chrome timeouts in several chunks, and devtools in browser_responsiveui.js.

Comment on attachment 8717535 [details] MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/3-4/

Comment on attachment 8717536 [details] MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/3-4/

Comment on attachment 8717537 [details] MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/3-4/

Comment on attachment 8717538 [details] MozReview Request: Bug 1245916: Add additional default globals. r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/3-4/

Comment on attachment 8717539 [details] MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/3-4/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/3-4/

Comment on attachment 8717537 [details] MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe I had to fix a couple of things so please review these small changes: https://reviewboard.mozilla.org/r/34203/diff/3-4/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer A couple of minor fixes here: https://reviewboard.mozilla.org/r/34209/diff/3-4/

Comment on attachment 8717540 [details] MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer https://reviewboard.mozilla.org/r/34209/#review31817

https://hg.mozilla.org/integration/fx-team/rev/ee408fd749ce https://hg.mozilla.org/integration/fx-team/rev/3381f46ec15b https://hg.mozilla.org/integration/fx-team/rev/44d78d157366 https://hg.mozilla.org/integration/fx-team/rev/c8abf68f9685 https://hg.mozilla.org/integration/fx-team/rev/5a5427be91e8 https://hg.mozilla.org/integration/fx-team/rev/f322e65d1069

https://hg.mozilla.org/mozilla-central/rev/ee408fd749ce https://hg.mozilla.org/mozilla-central/rev/3381f46ec15b https://hg.mozilla.org/mozilla-central/rev/44d78d157366 https://hg.mozilla.org/mozilla-central/rev/c8abf68f9685 https://hg.mozilla.org/mozilla-central/rev/5a5427be91e8 https://hg.mozilla.org/mozilla-central/rev/f322e65d1069